e2e/tests/regression/test_scheduling.robot (2)

17-22: Consider adding documentation for the variables.

While the variables are well-defined, adding documentation to explain their significance and chosen values would improve maintainability. For example, document why 300 retries with 1s interval was chosen for the retry parameters.

 *** Variables ***
+# Maximum number of test iterations
 ${LOOP_COUNT}    1
+# Maximum number of retries for async operations (5 minutes total with 1s interval)
 ${RETRY_COUNT}    300
+# Interval between retries in seconds
 ${RETRY_INTERVAL}    1
+# Data engine version to use for the volume
 ${DATA_ENGINE}    v1

---

40-53: Consider adding additional verifications for completeness.

While the test covers the core functionality, consider adding these verifications:

1. Verify node schedulability status after cordoning
2. Validate the replica count matches expectations

     And Cordon node 1
+    And Verify node 1 is unschedulable
     And Delete volume 0 replica on node 1
 
     Then Wait until volume 0 replicas rebuilding completed
     And Wait for volume 0 healthy
+    And Verify volume 0 has 3 replicas
     And Check volume 0 data is intact

e2e/keywords/k8s.resource (1)

72-75: Add documentation for the new keyword.

Consider adding documentation to describe the purpose and parameters of this keyword, following the Robot Framework documentation style.

+[Documentation]    Cordons a Kubernetes node specified by its index (0-based).
+...               The keyword will fail if the node cannot be cordoned or
+...               if the node remains schedulable after cordoning.
+[Arguments]    ${node_id}
 Cordon node ${node_id}
     ${node_name} =    get_node_by_index    ${node_id}
     cordon_node    ${node_name}
     check_node_is_not_schedulable    ${node_name}

e2e/libs/keywords/volume_keywords.py (1)

Line range hint 191-211: Add timeout and error handling to the async method.

While the implementation is correct, consider adding timeout and error handling for robustness:

         async def wait_for_both_replica_rebuildings():
             tasks = [
                 asyncio.create_task(self.volume.wait_for_replica_rebuilding_start(volume_name_0, node_id), name=volume_name_0),
                 asyncio.create_task(self.volume.wait_for_replica_rebuilding_start(volume_name_1, node_id), name=volume_name_1)
             ]
 
-            done, pending = await asyncio.wait(tasks, return_when=asyncio.ALL_COMPLETED)
-            logging(f"Observed {done.pop().get_name()} and {done.pop().get_name()} started replica rebuilding first")
+            try:
+                done, pending = await asyncio.wait(
+                    tasks,
+                    timeout=300,  # 5 minutes timeout
+                    return_when=asyncio.ALL_COMPLETED
+                )
+                if pending:
+                    for task in pending:
+                        task.cancel()
+                    raise TimeoutError(f"Timeout waiting for replica rebuilding to start")
+                logging(f"Observed {done.pop().get_name()} and {done.pop().get_name()} started replica rebuilding")
+            except Exception as e:
+                for task in tasks:
+                    task.cancel()
+                raise RuntimeError(f"Failed to wait for replica rebuilding: {str(e)}")

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Check for other cordon implementations and usage patterns
rg -A 5 "cordon_node|check_node_is_not_schedulable" --type robot